-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support for a generic SOF callback #2213
Conversation
I'm unsure if the device port should be part of the parameter of the callback or not. I started the PR with it but removed it for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sof must be explicitly enable/disable with additional API
I've move sof status into usbd_device_t to ensure it's cleared on bus reset or configuration change.
|
tud_sof_cb_enable() added for user land sof control.
_usbd_dev.sof_consumer &= (uint8_t)(~(1 << consumer)); | ||
} | ||
|
||
// Test logically unequal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a typo, should be: // Test logically XOR
But I don't get why to mention the specific implementation instead of the goal. While a hint that this is a logical XOR is indeed nice I think a comment should primary explain the meaning and not the implementation detail. Like: // Logically XOR to only handle a state change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a typo, it could be other consumers later, should be only called when whole value change to true or false.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
TU_ATTR_WEAK void tud_sof_cb(uint32_t frame_count) { | ||
(void)frame_count; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have a weak declaration in the header file? After all it's used only once at #L629 anyway. So a check there for a definition similar how it's done for other things should be more inline.
Edit:
In that case there can be a sanity check. Like to return false at #L394 if there is no function definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are converting to weak default implementation for keil and clang compatibility.
bool tud_sof_cb_enable(bool en) | ||
{ | ||
usbd_sof_enable(_usbd_rhport, SOF_CONSUMER_USER, en); | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A return value makes no sense here.
While also true for my original PR that wasn't as obvious. I wasn't sure if every driver has implemented dcd_sof_enable()
and so used TU_VERIFY()
to check for that.
It would make sense if there is a check if an implementation of a weak declared tud_sof_cb()
is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, forgot to change.
Can be used to synchronize the scanning to the polling for HID devices or other device functions. Not useful for time critical things because the time which has passed between the interrupt and the execution is unknown. For that it would be necessary to save the timestamp when the SOF interrupt handling is done in the ISR.
Proper example implementation of the PR #2180.
Edit:
A draft for a vendor specific SOF callback which is executed in the ISR via
dcd_event_handler()
can be found here: #2220